discards packets which are not (fully) populated #3508
Conversation
Not discarding packets which are not (fully) populated is bug prone or might waste resources.
| @@ -90,9 +90,11 @@ impl PacketBatch { | |||
| // break the payload into smaller messages, and here any errors | |||
| // should be propagated. | |||
| error!("Couldn't write to packet {:?}. Data skipped.", e); | |||
| packet.meta_mut().set_discard(true); | |||
There was a problem hiding this comment.
Idea makes sense, what do you think about moving line inside Packet::populate_packet so that we get this behavior across the board, not just from this one PacketBatch constructor ?
There was a problem hiding this comment.
I would rather not do that.
If there is a failure, Packet::populate_packet is returning the error to the caller. The caller may choose to either:
discardthe packet (like in this commit).- or, reuse the packet and write the next payload to it. In which case it shouldn't be marked
discard.
The problem with the current code is that the caller is effectively ignoring the error.
There was a problem hiding this comment.
or, reuse the packet and write the next payload to it. In which case it shouldn't be marked discard.
Sure, but if the caller is making a conscious decision to "recycle" the packet, it doesn't seem unreasonable to call .set_discard(false) to clean up this "dirtied" packet
There was a problem hiding this comment.
That would require the caller to know the implementation details Packet::populate_packet and what side effects it causes when it fails, which is generally not good.
The error returned by Packet::populate_packet however is part of the signature and explicit.
There was a problem hiding this comment.
I'm fine with this as-is; I'm probably lacking some context for the situation you have in mind.
On a related note, I'm currently working on a change to clean this up:
Lines 210 to 213 in 966c3c8
As part of that work, I'll likely be reviewing the various places packets are created & populated, so we can revisit incase I develop a stronger opinion on this
| @@ -90,9 +90,11 @@ impl PacketBatch { | |||
| // break the payload into smaller messages, and here any errors | |||
| // should be propagated. | |||
| error!("Couldn't write to packet {:?}. Data skipped.", e); | |||
| packet.meta_mut().set_discard(true); | |||
There was a problem hiding this comment.
or, reuse the packet and write the next payload to it. In which case it shouldn't be marked discard.
Sure, but if the caller is making a conscious decision to "recycle" the packet, it doesn't seem unreasonable to call .set_discard(false) to clean up this "dirtied" packet
Problem
Not discarding packets which are not (fully) populated is bug prone or might waste resources.
Summary of Changes
Discard packets which are not (fully) populated.